-
Notifications
You must be signed in to change notification settings - Fork 885
Fix selection in readOnly mode, Add magnifier via RawMagnifier widget #2529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
From what I see, this restores the magnifier behavior, right? We've had issues with this implementation before, and it led to some problems during rollbacks. For the reasons mentioned above, it's best to test this PR until we know it won't cause us problems, like it did last time. In any case, thank you for taking the time and effort to make these changes. |
This does restore magnifier behavior. I did not create the previous magnifier implementation, but I did fix some of it's bugs that had been merged. However, this is a completely different implementation that is using Flutter's RawMagnifier widget in the Widget tree (see text_selection.dart) instead of changing events / config / etc throughout the editor. The other dart changes in this PR are just passing a ValueNotifier around, and that ValueNotifier is only used in text_selection.dart. (This PR also has some selection fixes when in read only mode) I agree needs to be tested. I have an internal team of 8, 1.2 million app users creating just over 10K notes a day. They have been running the 10.8.5 version that had my previous magnifier fixes. Prior to the fixes, we received several support tickets about the magnifier bugs, since the fixes no reports. After we finish internal testing, we will do a rollout to our users and the feature will be used there. We have finished updating to flutter_quill 11.x and require the selection and magnification changes. |
…llMangifierBuilder, also added a defaultQuillMagnifierBuilder function, when not specified the magnifier will not be used.
|
This reverts commit 45844a3. Reasons to revert: * For full text selectable readonly editor contents, `SelectableText` only works on `TextSpan` children. `WidgetSpan` will make a gap for text selection and implementing a `SelectableWidgetSpan` combines both kinds of span is not easy to do: assert failure everywhere. * `SelectableArea` handles `onTap` event where we only want the gesture passed to children so that users could activate reply state by a single tap, to fix the issue we need a custom ignore pointer widget. * Rendered style not consists comparing the editor when editing and viewing page contents because the later one uses custom html parser while the former one uses editor which renders custom bbcode tags, they are not the same, both in history and in the future, as long as the process differs. And the html parser is quite limited and not stable, endless html layout rules are not clearly implemented, especially in line break. Frequently used layout are supported but not always be, massive special logic in the custom html parser makes it hard to maintain, let alone the selectable text feature we want to land in a future version. I think it's time to sunset the current html parser, or at least render into quill_delta so that we can have consistent contents appearance when editing and viewing contents. Next step: * Other implementation may not work, like use `Wrap` to simulate the original rich text features in flutter SDK, I don't think the result could be better than the one in SDK. * We need selectable text and consistent appearance when editing and viewing contents so render it in the bbcode editor is the best choice. Waiting for upstream fix selection in readOnly mode here: singerdmx/flutter-quill#2529 After that, parse supported tags here: https://github.com/realth000/tsdm_client/blob/81395ca8f5f5ee9ebe63cff1d223f585a895bf9a/lib/utils/html/html_muncher.dart#L335 into custom bbcode tags to let the bbcode editor render it as common quill_delta tags, hard but worth it.
Sorry for the delay, is the fix of selection in readOnly mode related to the PR #2488? |
@@ -853,8 +853,9 @@ class QuillRawEditorState extends EditorState | |||
}); | |||
} | |||
|
|||
controller.addListener(_didChangeTextEditingValueListener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looks like this PR removes the lines added by #2488 but keet some of the other changes, does this PR handle any related issues or conflicts with that PR? If no, then we need to revert that PR first, then merge this but revert the selection fix changes.
Read only mode needs the listener for selection changes. Only changes I see in my branch is controller.addListener(_didChangeTextEditingValueListener); and controller.removeListener(_didChangeTextEditingValueListener); are called in readonly mode. #2488 moves the handle focus change listener inside if (!widget.config.readOnly) {} - Could that have been the fix for focus changes bug? |
FYI - we've been internal testing with 20+ daily users for a couple of weeks with no issues. |
Thanks for your contribution. |
The default Android apps magnifier stays fixed to the text selection line, but this one moves freely with the finger's position. |
* master: (83 commits) Add copyWith methods to DefaultStyles, HorizontalSpacing, and VerticalSpacing (singerdmx#2550) docs: update migration guide to relfect singerdmx#2529 chore(release): prepare to publish 11.4.0 feat: allow to override link validation check, and accept mailto and other links by default (singerdmx#2525) chore(release): prepare to publish 11.3.0 Fix selection in readOnly mode, Add magnifier via RawMagnifier widget (singerdmx#2529) Clarify platform support of QuillEditorConfig.onKeyPressed (singerdmx#2542) chore(release): prepare to publish 11.2.0 feat: cache for toPlainText in Document to avoid unnecessary text computing (singerdmx#2482) chore(example): update deps chore(release): prepare to publish 11.1.2 fix: QuillEditor doesn't respect the system keyboard brightness by default on iOS (singerdmx#2522) chore(release): prepare to publish flutter_quill_test chore(release): prepare to publish 11.1.1 Chore: general improvements in flutter_quill_tests API (singerdmx#2512) Explicitly schedule frame on secondary click. (singerdmx#2507) chore(release): prepare to publish 11.1.0 fix: unpredictable endless loop of '_handleFocusChanged' in the phase of page route changing when editor is set to readonly. (singerdmx#2488) feat: Enable BoxDecoration for DefaultTextBlockStyle of header Attribute (singerdmx#2438) Expose Rule type so that Document.setCustomRules can be used (singerdmx#2484) ...
Description
Related Issues
Type of Change